Initial draft cap 63 #1587

SirTyson · 2024-12-10T22:01:12Z

This is the initial draft CAP for in-memory Soroban state and automatic entry restoration via InvokeHostFunctionOp. It depends on partial state archival from CAP-0062, but not full state archival of CAP-0057.

dmkozh

I haven't looked through the full CAP yet (and the semantics probably can be discussed separately), but I have some issues with the XDR that probably are easier to resolve now.

core/cap-0063.md

dmkozh · 2024-12-11T00:12:17Z

core/cap-0063.md

+     uint32 bucketListWriteFeeGrowthFactor;
+ };
+
+struct ConfigSettingContractLedgerCostExtV1


I don't have too strong opinion, but it might make sense to use V1 struct with all the fields instead of ext here. Or at least call it V0Ext to clarify the connection between these two.

dmkozh · 2024-12-11T00:16:06Z

core/cap-0063.md

+    // provided in the LedgerFootprint vectors. readOnly
+    // vector is first, followed by readWrite.
+    // If no keys are evicted, BitMap can be empty.
+    BitMap evictedSorobanEntries;


We don't need hacky indexing here because only RW entries can be restored.

Also, I've mentioned this already offline, I think we should have a vec of indices of entries that need to be restored. This will reduce the size of 99% of transactions as almost no transactions need to restore any entries. I think a slight increase in size for the txs that do restore entries is worth it. Also, it will be easier to interpret a sequence of indicies vs an opaque blob.

I agree with @dmkozh I think a separate list of just the evicted entries is easier to understand and will be more space efficient in the average case

Did a little digging and we're currently averaging 1.367 keys per restore op. I thought it would be way higher so I agree, vector of indices is better. Should we go uint8 or uint16? Do we ever forsee footprints going beyond 255?

I'd go for u16 just in case. The overhead is marginal, but I'd say accessing 255+ keys is not out of the realm of possibility Though restoring 255+ entries seems unlikely... Still, I feel like it might create unnecessary confusion in the future.

Are you planning to have 16-bit integer values in the XDR? XDR doesn't support a 16-bit integer type, the smallest integer is a 32-bit, because all values in XDR are padded to 4 bytes, so a 1 or 2 byte value would be 4 bytes anyway.

You can actually have a vector of 16 bits if you bitpack an opaque vector. For example, we do this in the fingerprints vector in SerializedBinaryFuseFilter. I've followed a similar spec here where I just use an opaque vector.

core/cap-0063.md

dmkozh · 2024-12-11T00:17:24Z

core/cap-0063.md

+    case 0:
+        void;
+    case 1:
+        SorobanResourcesExtV1 resourceExt;


What about the proofs, do we allocate an ext for these?

I didn't include it in this CAP just because they aren't necessary until full state archival.

core/cap-0063.md

tamirms · 2024-12-12T08:40:14Z

core/cap-0063.md

+This is necessary for determining what resources limits a Soroban key counts towards, but
+exposes a significant DOS angle. To prevent this attack, the footprint must statically declare which entries are live vs. evicted.
+
+This is accomplished via the `evictedSorobanEntries` vector. If any evicted Soroban entry is present in a TX's footprint,


what happens if you mark a ledger entry as evicted in the invoke host function operation but during application it turns out the ledger entry has already been restored? will the operation still succeed?

I assume that if a ledger entry is not present in evictedSorobanEntries and it turns out that the ledger entry is evicted, the transaction will fail regardless of whether there are enough fees to pay for the unexpected write. Is that correct?

Yeah, if you don't mark an entry as evicted you fail. If you do mark it as evicted and it's not, you're spending more in fees and resources, so we shouldn't fail. I'll update this section, but if an entry is marked evicted, we should charge it as if it's evicted no matter what, but not fail if it's actually live.

See added "Incorrect Entries in evictedSorobanEntries Vector" section.

dmkozh · 2024-12-12T19:51:38Z

core/cap-0063.md

+    // Vector of uint16_t indices representing what Soroban
+    // entries in the footprint are evicted, based on the
+    // order of keys provided in the readWrite footprint.
+    opaque<> evictedSorobanEntries;


TBH for the sake of interpretability I'd probably just have these as a normal vector of ints.

anupsdf · 2024-12-17T23:02:02Z

core/cap-0063.md

+
+- in-memory entries:
+  - Live Soroban entries
+  - Archived entries that have not yet been evicted (i.e. still exist in the live BucketList)


Should we say expired entries here instead of archived?

anupsdf · 2024-12-18T17:56:14Z

core/cap-0063.md

+ };
+
+// Ledger access settings for contracts.
+struct ConfigSettingContractLedgerCostV1


Is it possible to split limits and fees in different structs? That way the one with fees can have cost in the name (ConfigSettingContractLedgerCost) while the other one could be named ConfigSettingContractLedgerLimitV1.

(Probably a question for @dmkozh)

Originally I did that, but Dima suggested one struct instead (I unresolved the comment above for context). I don't care particularly strongly either way.

Is it possible to split limits and fees in different structs?

We didn't do that initially, and I don't think it makes much sense to change this now. Conceptually the 'cost' here concerns both resource limits ('cost' for the validators) and fees ('cost' for the user).

Originally I did that, but Dima suggested one struct instead

FWIW I suggested to use one struct for the setting version migration, this wasn't about separating the fees and limits. But this is the first time we do a setting update and there is no well-defined process yet. Giving this a second thought, I think going the extension way is probably less disruptive for anyone who consumes these settings, as having two versions introduces the risk of using the deprecated value. The name should be ConfigSettingContractLedgerCostV0Ext then. Version bumps should probably be reserved for non-incremental and truly breaking changes. Sorry for the confusion, I should have thought more about this initially.

anupsdf · 2024-12-19T18:31:43Z

core/cap-0063.md

+`LEDGER_ENTRY_STATE` followed by the change. This is similar, except that `LEDGER_ENTRY_STATE` is replaced
+by `LEDGER_ENTRY_RESTORE`.
+
+#### Deprecation of RestoreFootprintOp(?)


Worth asking the ecosytem on deprecating this Op. Only thing I can think of is that if admin doesn't want the users to incur the restoration fees and keeps expired/archived entries restored on behalf of the users.

anupsdf · 2024-12-19T18:34:34Z

core/cap-0063.md

+
+Used to query both live and archived LedgerEntries. While `getledgerentryraw` does simple key-value lookup
+on the live ledger, `getledgerentry` will query a given key in both the live BucketList and Hot Archive BucketList.
+It will also report whether an entry is archived, evicted, or live, and return the entry's current TTL value.


Should we refer to the states as live, expired, archived? This way expired indicates those still in live BL but not evicted and archived refers to those in hot archive BL.

anupsdf · 2024-12-20T00:05:04Z

core/cap-0063.md

+- `state`: One of the following values:
+  - `live`: Entry is live.
+  - `new`: Entry does not exist. Either the entry has never existed or is an expired temp entry.
+  - `archived`: Entry is archived, but not yet evicted, counts towards in-memory resources.


Same thought here if we should call expired for entries in live BL and archived for entries that are evicted from live and are in hot archive.

This way archived entries count towards disk resources while expired ones are in-memory.

anupsdf · 2024-12-20T00:14:07Z

core/cap-0063.md

+TX would consume little resources and would be cheap, but would load the maximum amount of in-memory state
+and avoid these implicit fees. Due to the low cost of these loads, it seems unlikely this would negatively
+affect the network. Even the TX size costs associated with the large footprint may make the attack economically
+nonviable relative to the amount of stress on the network. However, if this were to become and issue,


typo: an issue

anupsdf · 2024-12-20T00:20:30Z

core/cap-0063.md

+    // Vector of uint16_t indices representing what Soroban
+    // entries in the footprint are evicted, based on the
+    // order of keys provided in the readWrite footprint.
+    opaque<> evictedSorobanEntries;


Should we call this archivedEntries dropping Soroban in the name since its already inside _Soroban_ResourcesExtV0 struct?

anupsdf · 2024-12-20T00:23:22Z

core/cap-0063.md

+disk resources when no entries are evicted. Should any Soroban entry be evicted, automatic
+restoration would not be possible, as the restoration would exceed resource limits. In this case, it would be
+necessary to manually issue a `RestoreFootprintOp` (or equivalent in the case of deprecation) prior to the
+host function invocation.


One reason to not deprecated RestoreFootprintOp.

anupsdf · 2024-12-20T00:33:02Z

core/cap-0063.md

+However, this is not a significant concern. The `sorobanLiveStateTargetSizeBytes` (see [CAP-0062](cap-0062.md))
+provides a soft cap on the amount of live Soroban state that can exist at any given time. This prevents any
+sort of runaway memory issue. Additionally, because of the eviction process, there is a natural back pressure
+applied to in-memory state, the rate at which can be controlled via network config settings. Finally, the rate


Maybe we should highlight this up top that background eviction will remove in-memory Soroban entries.

Initial draft cap 63

fa46ef3

SirTyson requested review from dmkozh, anupsdf and MonsieurNicolas December 10, 2024 22:01

dmkozh reviewed Dec 11, 2024

View reviewed changes

tamirms reviewed Dec 11, 2024

View reviewed changes

core/cap-0063.md Show resolved Hide resolved

tamirms reviewed Dec 11, 2024

View reviewed changes

core/cap-0063.md Show resolved Hide resolved

tamirms reviewed Dec 11, 2024

View reviewed changes

core/cap-0063.md Show resolved Hide resolved

Addressed initial feedback

a6d9034

tamirms reviewed Dec 12, 2024

View reviewed changes

Updated malformed evictedSorobanEntries section

ddff828

dmkozh reviewed Dec 12, 2024

View reviewed changes

anupsdf reviewed Dec 17, 2024

View reviewed changes

anupsdf reviewed Dec 18, 2024

View reviewed changes

anupsdf reviewed Dec 19, 2024

View reviewed changes

anupsdf reviewed Dec 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial draft cap 63 #1587

Initial draft cap 63 #1587

SirTyson commented Dec 10, 2024

dmkozh left a comment

dmkozh Dec 11, 2024

dmkozh Dec 11, 2024

tamirms Dec 11, 2024

SirTyson Dec 11, 2024

dmkozh Dec 11, 2024

leighmcculloch Dec 12, 2024

SirTyson Dec 12, 2024

dmkozh Dec 11, 2024

SirTyson Dec 11, 2024

tamirms Dec 12, 2024

SirTyson Dec 12, 2024

SirTyson Dec 12, 2024

dmkozh Dec 12, 2024

anupsdf Dec 17, 2024

anupsdf Dec 18, 2024

SirTyson Dec 18, 2024

dmkozh Dec 18, 2024

anupsdf Dec 19, 2024

anupsdf Dec 19, 2024

anupsdf Dec 20, 2024

anupsdf Dec 20, 2024

anupsdf Dec 20, 2024

anupsdf Dec 20, 2024

anupsdf Dec 20, 2024

anupsdf Dec 20, 2024 •

edited

Loading

Initial draft cap 63 #1587

Are you sure you want to change the base?

Initial draft cap 63 #1587

Conversation

SirTyson commented Dec 10, 2024

dmkozh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anupsdf Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

anupsdf Dec 20, 2024 •

edited

Loading